Review

51K additions — this is a very large PR. It stacks on #67 (which stacks on #66), so all issues from both prior reviews carry forward. The new commit adds ~39K lines, of which ~28K are widget template JSON files.

Scope

This should be at least 3 separate PRs:

1. Widget template JSON files (20 new templates) — bulk addition, easy to review separately
2. Pluggable widget engine v2 (widget_engine.go, augment.go, loader.go, visitor/executor changes)
3. Widget CLI commands (cmd_widget.go)

Bundling makes this effectively unreviewable at 51K lines.

Code quality issues

1. Silent nil return in deepCloneMap (augment.go)

deepCloneMap serializes to JSON then deserializes back. On error, it returns nil without signaling the caller. This causes cascading nils — cloned properties silently become nil, and downstream code gets mysterious failures instead of a clear error.

2. Unsafe type assertions throughout

Multiple places assume BSON structure shapes without checking. For example, createAttributeObject silently returns nil AttributeRef when the attribute path has fewer than 2 dots, instead of returning an error for invalid paths.

3. Entity context mutation (widget_engine.go)

entityContext is saved/restored around Build() but mutated in-place during DataSource mapping. If an error occurs partway through, the context is left corrupted. Should use an immutable context stack or pass context as a parameter.

4. Placeholder ID generation (augment.go)

Uses atomic.AddInt64 for a counter but ResetPlaceholderCounter() exists for testing — this is a race condition if tests run in parallel. The deterministic format also means IDs may collide across concurrent augmentations.

5. Mode selection swallows ambiguity (widget_engine.go)

When multiple modes have no condition, the first fallback is silently chosen with no warning. This hides accidental duplicate mode definitions that could produce wrong widget output.

6. Debug logging still present from PR #66

The log.Printf("SERIALIZE CHECK: timelineCustom Widgets: ...") debug code in writer_widgets_custom.go is still here from the base commits.

Missing error handling

• buildAttributeObjects: No validation that the resolved attribute path actually exists in the domain model before creating a reference
• Operations in widget_engine.go (opAssociation, etc.): Check if source is empty but don't validate the resolved context (e.g., EntityName being set)
• containsPlaceholderID gives generic error messages — doesn't indicate which field has the unreplaced placeholder

Design concerns

JSON round-trip for deep clone

Using JSON marshal/unmarshal for deep cloning BSON structures is fragile and has performance implications. Custom BSON types or binary data won't survive the round-trip. A proper BSON deep-clone would be more robust.

Tight coupling to BSON

PluggableWidgetEngine receives and returns bson.D directly with no abstraction. This makes the engine untestable without real BSON structures and means any serialization format changes require rewriting the entire module.

Testing

• sdk/mpr/roundtrip_test.go is a good addition (+212 lines) — this addresses the gap flagged in PR #66 review
• However: no tests for deepCloneMap error paths, no tests for partial operation failures, no tests for mode fallback ambiguity, no tests for placeholder ID uniqueness under concurrency

Design docs committed

docs/plans/2026-03-30-widget-docs-and-customwidget.md and docs/plans/2026-03-30-widget-docs-generation-design.md are committed. Are these intended to live permanently, or should they be removed post-implementation? They describe planned work rather than documenting what was built.

Template files

20 new widget templates added under sdk/widgets/templates/mendix-11.6/. These are extracted from Studio Pro which is the right approach per the repo's guidelines. However:

• Were all templates verified to include both type AND object fields as required by sdk/widgets/templates/README.md?
• timeline.json shows +0 -0 (empty file?) — needs verification

Summary

The core widget engine design is reasonable, but the PR needs:

1. Splitting into reviewable units
2. Error handling hardened (especially deepCloneMap, attribute validation, context mutation)
3. Debug logging removed
4. Tests for error paths
5. All issues from PR #66 and #67 reviews resolved first

Review (updated — 3 new fix commits since last review)

Good progress addressing the issues from my earlier review. Here's what changed and what remains.

Issues addressed

Remaining issues

1. ANTLR visitor files still generated (+2,635 lines)

mdlparser_base_visitor.go and mdlparser_visitor.go are still added. These don't exist on main and aren't consumed by any code. Was the ANTLR config intentionally changed to generate visitors, or is this accidental?

Also: MDLLexer.interp, MDLLexer.tokens, MDLParser.interp, MDLParser.tokens show as deleted (-4,123 lines). These are normally part of the generated parser output. Were they intentionally removed? This may break ANTLR tooling that relies on them.

2. opAttributeObjects swallows errors

attrObj, _ := ctx.pageBuilder.createAttributeObject(...)
if attrObj != nil {
    objects = append(objects, attrObj)
}

createAttributeObject now returns errors (good), but the caller discards them with _. If an attribute path is invalid, the attribute is silently skipped instead of reporting the error to the user.

3. deepCloneTemplate error ignored in tests

clone, _ := deepCloneTemplate(original)

Three test sites ignore the error from deepCloneTemplate. Tests should check errors — t.Fatal(err) if it fails.

4. augmentFromMPK silently falls back

clone, err := deepCloneTemplate(tmpl)
if err != nil {
    return tmpl  // silently returns unaugmented template
}

This is better than nil, but a clone failure means the template may be mutated later (since augmentation modifies in-place). Consider logging the error.

5. Design docs still committed

docs/plans/2026-03-30-widget-docs-and-customwidget.md and docs/plans/2026-03-30-widget-docs-generation-design.md are still present. Are these intended as permanent documentation?

6. Still too large for practical review

50K additions across 69 files. The widget template JSONs alone are ~28K lines. Splitting templates into a separate PR would make this much more reviewable.

New code quality — generateDefJSON (commits 2-3)

The auto-generation of property mappings from MPK definitions is a nice improvement. The two-pass approach for association ordering is clean and well-tested. One minor issue:

case "boolean", "integer", "decimal", "string", "enumeration":
    m := executor.PropertyMapping{
        PropertyKey: p.Key,
        Operation:   "primitive",
    }

This maps "enumeration" to "primitive" operation — is that correct? Enumerations in Mendix are typically not simple primitives. If the primitive operation just sets a string value, it may work, but the naming is misleading.

Summary

The fix commits address the most critical issues (error handling, debug logging, code duplication). The remaining concerns are the ANTLR visitor files, swallowed errors in opAttributeObjects, and PR size. The core engine changes look solid.